[sql-33] session: add migration code from kvdb to SQL #1051

ViktorTigerstrom · 2025-04-29T17:47:35Z

This PR introduces the migration logic for transitioning the sessions store from kvdb to SQL.

Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

Part of #917

ViktorTigerstrom

Adding some review comments where input would be extra helpful :)!

ViktorTigerstrom · 2025-05-02T09:14:00Z

session/sql_store.go

+		macRecipe = &MacaroonRecipe{}
+		if perms != nil {
+			macRecipe.Permissions = unmarshalMacPerms(perms)
+		}
+		if caveats != nil {
+			macRecipe.Caveats = unmarshalMacCaveats(caveats)


Let me know if you think we should rather fix the kvdb store implementation rather than the sql one. But as we've previously opted for not updating the store behaviour, and make the sql one mimic the behaviour until we've completed the migration, I opted for this version here.

Alternatively, I could also override this in newly added overrideMacaroonRecipe function before the migration deep equals check. But I kept this as is, as I do think it's correct behaviour that we don't set the Permissions or Caveats and keep them at nil, if no value actually exists for them.

i think in this case it is fine to fix on the kvdb side so that our sql side can stay neat?

also commit message says fix kvdb&sql but this only touches sql

ViktorTigerstrom · 2025-05-02T09:17:17Z

session/sql_migration.go

+	if !session.RevokedAt.IsZero() {
+		err = tx.SetSessionRevokedAt(
+			ctx,
+			makeSetRevokedAtParams(sqlId, session.RevokedAt),
+		)
+		if err != nil {
+			return err
+		}
+	}


Could potentially update the insert session sql query to also take an optional RevokedAt sql.NullTime param, to not have to do this as a separate step. I opted for this version to keep things as is as much as possible, but if you think that'd be better, I'll update to do that instead :)

ViktorTigerstrom · 2025-05-02T09:20:45Z

session/sql_migration_test.go

+		{
+			"randomized sessions",
+			randomizedSessions,
+		},


Opted for a test that uses the "standard" rand lib here, as @bitromortac had a preference for that in #1047 (comment). But can also look into adding a test that uses rapid if you'd prefer.

ViktorTigerstrom · 2025-05-02T09:24:08Z

session/sql_migration_test.go

+			// We always have a caveat.Id, but the rest are
+			// randomized if they exist or not.
+			macCaveat.Id = randomBytes(rand.Intn(10) + 1)


Let me know if this is not true in for real world data, as migrations WILL fail if we don't set a caveat.Id when the caveats param of WithMacaroonRecipe is not nil.

Could potentially override this as well in the new overrideMacaroonRecipe as well. But I do not think this scenario can happen for real world data. But open to implementing that as well if you think it's worth doing it just to be sure.

ViktorTigerstrom · 2025-05-02T09:25:33Z

session/sql_migration_test.go

+// "dest" state, without checking if the state transition is legal.
+//
+// NOTE: this function should only be used for testing purposes.
+func (db *BoltStore) shiftStateUnsafe(_ context.Context, id ID,


Not fully sure if we add test functions like this one that's a struct member to the _test file, or if we add it in the actual struct file?

right, not too great, you could change to func shiftStateUnsafe(_ context.Context, db *BoltStore, id ID, dest State) error {

bitromortac

Great work, very clean on first pass! 🔥

bitromortac · 2025-05-07T13:16:17Z

session/sql_migration.go

+	// The remote public key is currently only set for autopilot sessions,
+	// else it's an empty byte array.


nit: I think it should read else it's a nil byte array, since we compare to nil in the code in other sites, which is also correctly set above with the var statement

session/sql_migration_test.go

bitromortac · 2025-05-07T13:55:11Z

session/sql_migration_test.go

+			require.NoError(t, err)
+
+			t.Cleanup(func() {
+				require.NoError(t, kvStore.DB.Close())


tiny nit: the embedded DB can be removed

bitromortac · 2025-05-07T13:57:31Z

session/sql_migration_test.go

+		// numberOfSessions is set to 100 to add enough sessions to get
+		// enough variation between number of invoices and payments, but
+		// kept low enough for the test not take too long to run, as the
+		// test time increases drastically by the number of sessions we
+		// migrate.


the doc mentions invoice and payments, which is probably a copy/paste

bitromortac · 2025-05-07T14:13:11Z

session/sql_migration_test.go

+// "dest" state, without checking if the state transition is legal.
+//
+// NOTE: this function should only be used for testing purposes.
+func (db *BoltStore) shiftStateUnsafe(_ context.Context, id ID,


right, not too great, you could change to func shiftStateUnsafe(_ context.Context, db *BoltStore, id ID, dest State) error {

ellemouton

holding off on review here until the prep PR discussed in the accounts mig PR is in (since that will be used to test this one)

ViktorTigerstrom · 2025-05-19T19:32:10Z

Now that #1047 has been merged, I'll update this PR to adhere to the feedback brought up in that PR, and then re-request reviews afterwards.

In preparation for upcoming migration tests from a kvdb to an SQL store, this commit updates the NewTestDB function to return the Store interface rather than a concrete store implementation. This change ensures that migration tests can call NewTestDB under any build tag while receiving a consistent return type.

lightninglabs-deploy · 2025-05-26T20:21:40Z

@ViktorTigerstrom, remember to re-request review from reviewers when ready

When a session has a MacaroonRecipe set, which has a nil value set for either the `Permissions` or the `Caveats` field, the kvdb session store would return that field as nil, while the sql prior to this commit would return an empty array. When the we migrate the session store from kvdb to sql, that will cause the kvdb and the migrate session store to differ when comparing them, which will cause the migration to fail. This commit fixes that by ensuring that the sql session store returns a nil value for those fields in that scenario, rather than an empty array.

ViktorTigerstrom · 2025-05-27T11:04:22Z

Updated the PR to also address feedback raised in #1047! Will address the small linting issue after next review round :)

bitromortac

LGTM, utACK 🎉

bitromortac · 2025-05-27T12:33:21Z

session/sql_migration.go

+				},
+			}
+
+			err = tx.InsertSessionMacaroonCaveat(


nit: fits on single line, same for InsertSessionPrivacyFlagParams, I found this in other places as well, where you could contract a bit more

Thanks! I addressed that + other places that I found, so I hope this has been addressed now.

session/sql_migration_test.go

ViktorTigerstrom

Thanks a lot for the review @bitromortac 🎉!! I noticed that we can have real world data that sets a non-nil macaroon recipe, but with nil perms& caveats. I updated the code to be able to handle that case as well.

ViktorTigerstrom · 2025-05-28T11:49:50Z

session/sql_migration_test.go

+			},
+		},
+		{
+			name: "macaroon recipe with nil perms and caveats",


Note that this test is new, as this can actually happen for real world sessions.

ViktorTigerstrom · 2025-05-28T11:52:44Z

session/sql_store.go

+		macRecipe = &MacaroonRecipe{}
+		if perms != nil {
+			macRecipe.Permissions = unmarshalMacPerms(perms)
+		}
+		if caveats != nil {
+			macRecipe.Caveats = unmarshalMacCaveats(caveats)


Alternatively, I could also override this in newly added overrideMacaroonRecipe function before the migration deep equals check. But I kept this as is, as I do think it's correct behaviour that we don't set the Permissions or Caveats and keep them at nil, if no value actually exists for them.

ViktorTigerstrom · 2025-05-28T11:54:43Z

session/sql_migration_test.go

+			// We always have a caveat.Id, but the rest are
+			// randomized if they exist or not.
+			macCaveat.Id = randomBytes(rand.Intn(10) + 1)


Could potentially override this as well in the new overrideMacaroonRecipe as well. But I do not think this scenario can happen for real world data. But open to implementing that as well if you think it's worth doing it just to be sure.

This commit introduces the migration logic for transitioning the sessions store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

ellemouton

looking good!

ellemouton · 2025-05-28T13:10:17Z

session/sql_store.go

+		macRecipe = &MacaroonRecipe{}
+		if perms != nil {
+			macRecipe.Permissions = unmarshalMacPerms(perms)
+		}
+		if caveats != nil {
+			macRecipe.Caveats = unmarshalMacCaveats(caveats)


i think in this case it is fine to fix on the kvdb side so that our sql side can stay neat?

ellemouton · 2025-05-28T13:10:37Z

session/sql_store.go

+		macRecipe = &MacaroonRecipe{}
+		if perms != nil {
+			macRecipe.Permissions = unmarshalMacPerms(perms)
+		}
+		if caveats != nil {
+			macRecipe.Caveats = unmarshalMacCaveats(caveats)


also commit message says fix kvdb&sql but this only touches sql

ellemouton · 2025-05-28T13:12:40Z

session/sql_migration.go

+	// If sessions are linked to a group, we must insert the initial session
+	// of each group before the other sessions in that group. This ensures
+	// we can retrieve the SQL group ID when inserting the remaining
+	// sessions. Therefore, we first insert all initial group sessions,


interesting - are we not able to rely on the fact that sessions are stored in order in kvdb? ie, if we get a session with a group ID set, that will always either point to the session at hand or a session we've already handled?

ie, i think we can just:

for s := sessions { if s.groupID = s.ID { <insert new session. no coupled group> continue } groupID := db.GetGroupIDByAlias(s.groupID) <insert new session. couple to group> }

or did you actually run into a scenario where these were not in order?

ellemouton · 2025-05-28T13:20:26Z

session/sql_migration.go

+	session.AccountID.WhenSome(func(alias accounts.AccountID) {
+		// Check that the account exists in the SQL store, before


technically what we are doing here is just getting the account DB level ID.
we dont actually need to "check before link" since SQL does that for us.

ellemouton · 2025-05-28T13:20:45Z

session/sql_migration.go

+			return
+		}
+
+		acctID = sql.NullInt64{


sqldb.SQLInt64(accDBID)

ellemouton · 2025-05-28T13:22:37Z

session/sql_migration.go

+	if !session.RevokedAt.IsZero() {
+		setSessionRevokedParams := sqlc.SetSessionRevokedAtParams{
+			ID: sqlId,
+			RevokedAt: sql.NullTime{


sqldb.SQLTime

ellemouton · 2025-05-28T13:22:59Z

session/sql_migration.go

+	// Since the InsertSession query doesn't support that we set the revoked
+	// field during the insert, we need to set the field after the session
+	// has been created.


ellemouton · 2025-05-28T13:24:50Z

session/sql_migration.go

+	// session itself, and therefore the SQL id for the session won't exist
+	// prior to inserting the session.


ellemouton · 2025-05-28T13:26:02Z

session/sql_migration.go

+	// Now lets set the group ID for the session.
+	err = tx.SetSessionGroupID(ctx, sqlc.SetSessionGroupIDParams{
+		ID: sqlId,
+		GroupID: sql.NullInt64{


sqldb.SQLInt64

ellemouton · 2025-05-28T13:27:12Z

session/sql_migration.go

+		privacyFlagParams := sqlc.InsertSessionPrivacyFlagParams{
+			SessionID: sqlId,
+			Flag:      int32(privacyFlag),
+		}
+
+		err = tx.InsertSessionPrivacyFlag(ctx, privacyFlagParams)
+		if err != nil {
+			return err


this and many of the above can just be in-lined instead of having separate param vars

ViktorTigerstrom added the no-changelog This PR is does not require a release notes entry label Apr 29, 2025

ViktorTigerstrom force-pushed the 2025-04-migrate-sessions branch from 449a2cc to 1828ec4 Compare April 29, 2025 17:52

levmi assigned ViktorTigerstrom May 1, 2025

ViktorTigerstrom force-pushed the 2025-04-migrate-sessions branch from 1828ec4 to a868a93 Compare May 2, 2025 09:04

ViktorTigerstrom commented May 2, 2025

View reviewed changes

ViktorTigerstrom requested review from ellemouton and bitromortac May 6, 2025 09:56

bitromortac reviewed May 7, 2025

View reviewed changes

ellemouton reviewed May 8, 2025

View reviewed changes

ViktorTigerstrom added 2 commits May 27, 2025 12:07

session: Add Close function to Store interface

dfd0931

ViktorTigerstrom force-pushed the 2025-04-migrate-sessions branch from a868a93 to 4b3b39f Compare May 27, 2025 10:09

ViktorTigerstrom requested review from ellemouton and bitromortac May 27, 2025 11:04

bitromortac approved these changes May 27, 2025

View reviewed changes

ViktorTigerstrom mentioned this pull request May 27, 2025

[sql-41] firewalldb: add migration code for kvstores from kvdb to SQL #1079

Open

ViktorTigerstrom force-pushed the 2025-04-migrate-sessions branch 2 times, most recently from a48e9b2 to 8c970bf Compare May 28, 2025 11:46

ViktorTigerstrom commented May 28, 2025

View reviewed changes

ViktorTigerstrom added 2 commits May 28, 2025 14:45

session: add migration code from kvdb to SQL

04559fe

This commit introduces the migration logic for transitioning the sessions store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

session: add randomized session migration test

46873cb

ViktorTigerstrom force-pushed the 2025-04-migrate-sessions branch from 8c970bf to 46873cb Compare May 28, 2025 12:45

ellemouton reviewed May 28, 2025

View reviewed changes

		// The remote public key is currently only set for autopilot sessions,
		// else it's an empty byte array.

		session.AccountID.WhenSome(func(alias accounts.AccountID) {
		// Check that the account exists in the SQL store, before

		// session itself, and therefore the SQL id for the session won't exist
		// prior to inserting the session.

[sql-33] session: add migration code from kvdb to SQL #1051

Are you sure you want to change the base?

[sql-33] session: add migration code from kvdb to SQL #1051

Uh oh!

Conversation

ViktorTigerstrom commented Apr 29, 2025

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom commented May 19, 2025

Uh oh!

lightninglabs-deploy commented May 26, 2025

Uh oh!

ViktorTigerstrom commented May 27, 2025

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment